Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TargetVmSize, AvSet, Sql RP and ResourceTagging changes for V2A and H2A #21

Merged

Conversation

vidyadharijami
Copy link

@vidyadharijami vidyadharijami commented Mar 25, 2021

This PR includes changes for following for both V2A and H2A

TargetVmSize in enable protection,
AvSet in enable protection
Sql License type in enable, update and get APIs
ResourceTagging [VmTags, NicTags and DiskTags] in enable, update and get APIs
Design PRs:
Azure/azure-powershell-cmdlet-review-pr#879
Azure/azure-powershell-cmdlet-review-pr#888

Testing:
H2A is done.
V2A is done.

Added test cases and recordings for V2A and H2A

@@ -15,8 +15,8 @@ Enables replication for an ASR protectable item by creating a replication protec
### EnterpriseToEnterprise (Default)
```
New-AzRecoveryServicesAsrReplicationProtectedItem [-VmmToVmm] -ProtectableItem <ASRProtectableItem>
-Name <String> -ProtectionContainerMapping <ASRProtectionContainerMapping> [-WaitForCompletion]
[-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]
-Name <String> -ProtectionContainerMapping <ASRProtectionContainerMapping> [-UseManagedDisk <String>]
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UseManagedDisk [](start = 78, length = 14)

This is E2E right??? #Resolved

Copy link
Author

@vidyadharijami vidyadharijami Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove from here. It's by overlook. #Resolved

[-RecoveryVmTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-DiskTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-RecoveryNicTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-UseManagedDisk <String>] [-WaitForCompletion] -DiskType <String> [-DiskEncryptionSetId <String>]
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[-UseManagedDisk ] [](start = 1, length = 26)

Do we want to expose in V2A??? The SRS decides on its own right based on Geo
Also in this we have DiskType so this shd be MD only #Resolved

Copy link
Author

@vidyadharijami vidyadharijami Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In powershell also, we don't expose this for V2A. Will remove it. It has come from auto generation of help file. #Resolved

[-RecoveryVmTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-DiskTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-RecoveryNicTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-UseManagedDisk <String>] [-WaitForCompletion] [-DiskEncryptionSetId <String>]
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UseManagedDisk [](start = 3, length = 14)

Please check @anmolbhatia289 #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't expose this in V2A. Hence removing this.


In reply to: 601410343 [](ancestors = 601410343)

[-RecoveryVmTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-DiskTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-RecoveryNicTag <System.Collections.Generic.IDictionary`2[System.String,System.String]>]
[-WaitForCompletion] [-DiskEncryptionSetId <String>]
[-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]
```

### EnterpriseToAzure
```
New-AzRecoveryServicesAsrReplicationProtectedItem [-HyperVToAzure] -ProtectableItem <ASRProtectableItem>
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HyperVToAzure [](start = 52, length = 13)

Actually E2A also we have all right? then why this split #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing this as discussed.


In reply to: 601427963 [](ancestors = 601427963)

Type: System.String
Parameter Sets: VMwareToAzureWithDiskType, VMwareToAzure, HyperVSiteToAzure
Aliases:
Accepted values: NoLicenseType, PAYG, AHUB
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoLicenseType [](start = 17, length = 13)

I remember 4 values in SRS #Resolved

Copy link
Author

@vidyadharijami vidyadharijami Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't considered the NotSpecified case because this parameter itself won't be given if the user doesn't want to specify.
Similar to LicenseType which is already present. #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to double check with Prachetos on how is NoLicenseType different from NotSpecified.


In reply to: 601548721 [](ancestors = 601548721)

Type: System.String
Parameter Sets: (All)
Aliases:
Accepted values: NoLicenseType, PAYG, AHUB
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoLicenseType [](start = 17, length = 13)

Same as above #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will cross check on this and will add in next PR if any change required.


In reply to: 601541382 [](ancestors = 601541382)

@@ -864,6 +868,25 @@ public ASRHyperVReplicaAzureSpecificRPIDetails(HyperVReplicaAzureReplicationDeta
/// </summary>
public string RecoveryProximityPlacementGroupId { get; set; }

/// <summary>
/// Gets or sets the SQL Server license type to the machine to in the event of a failover.
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

machine to [](start = 60, length = 10)

Please correct the description, something like
Gets or sets the SQL Server license type of the machine in the event of a failover. #Resolved

Copy link
Author

@vidyadharijami vidyadharijami Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


In reply to: 601542526 [](ancestors = 601542526)

@@ -1182,6 +1209,26 @@ public ASRInMageAzureV2SpecificRPIDetails(InMageAzureV2ReplicationDetails detail
/// Gets or sets the proximity placement group Id for replication protected item after failover.
/// </summary>
public string RecoveryProximityPlacementGroupId { get; set; }

/// <summary>
/// Gets or sets the SQL Server license type to the machine to in the event of a failover.
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ts the SQL Server license type to the [](start = 22, length = 37)

Same as above #Resolved

Copy link
Author

@vidyadharijami vidyadharijami Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. #Resolved

[Parameter(ParameterSetName = ASRParameterSets.HyperVSiteToAzure, HelpMessage = "Specify the SQL Server license type of the VM.")]
[ValidateNotNullOrEmpty]
[ValidateSet(
Constants.NoLicenseType,
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoLicenseType [](start = 22, length = 13)

Same as above #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving as discussed.


In reply to: 601544871 [](ancestors = 601544871)

SqlServerLicenseType = this.SqlServerLicenseType,
TargetVmTags = this.RecoveryVmTag,
TargetNicTags = this.RecoveryNicTag,
SeedManagedDiskTags = this.DiskTag,
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SeedManagedDiskTags = this.DiskTag [](start = 16, length = 34)

If RecoveryAzureStorageAccountId is not null or string.empty, then also we shd set this. Otherwise service will throw exception.
@anmolbhatia289 #Resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct


In reply to: 601546593 [](ancestors = 601546593)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled in powershell itself.


In reply to: 601546593 [](ancestors = 601546593)

providerSettings.TargetVmSize = this.Size;
providerSettings.SqlServerLicenseType = this.SqlServerLicenseType;
providerSettings.TargetVmTags = this.RecoveryVmTag;
providerSettings.TargetManagedDiskTags = this.DiskTag;
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

providerSettings.TargetManagedDiskTags = this.DiskTag; [](start = 12, length = 54)

Only if useManagedDisks is true else service will throw error.
Should we throw error in powershell or we want to surface the srs exception?? #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled in powershell


In reply to: 601548064 [](ancestors = 601548064)

[Parameter]
[ValidateNotNullOrEmpty]
[ValidateSet(
Constants.NoLicenseType,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoLicenseType [](start = 22, length = 13)

Same as above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check on this once if we need to consider NotSpecified.


In reply to: 601549109 [](ancestors = 601549109)

var sqlServerLicenseType = this.SqlServerLicenseType;
var recoveryVmTag = this.RecoveryVmTag;
var recoveryNicTag = this.RecoveryNicTag;
var diskTag = this.DiskTag;
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiskTag [](start = 35, length = 7)

What if for H2A useManagedDisk is false? SRS will throw exception so are we fine with that? or we want to throw from powershell #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled in powershell.


In reply to: 601551907 [](ancestors = 601551907)

TargetProximityPlacementGroupId = proximityPlacementGroupId,
SqlServerLicenseType = sqlServerLicenseType,
TargetVmTags = recoveryVmTag,
TargetManagedDiskTags = diskTag,
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

argetManagedDiskTags = diskTa [](start = 33, length = 29)

question as above #Resolved

$diskTag.Add("DiskTag1","powershellDisk")
$nicTag = New-Object "System.Collections.Generic.Dictionary``2[System.String,System.String]"
$nicTag.Add("NicTag1","powershellNic")
$EnableDRjob = New-AsrReplicationProtectedItem -ProtectableItem $VM -Name $VM.Name -ProtectionContainerMapping $pcm -RecoveryAzureStorageAccountId $StorageAccountID -OSDiskName $VMName -OS Windows -RecoveryResourceGroupId $RecoveryResourceGroupId -RecoveryProximityPlacementGroupId $ppg -UseManagedDisk true -RecoveryAvailabilitySetId $avset -Size $size -SqlServerLicenseType $sqlLicenseType -RecoveryVmTag $vmTag -RecoveryNicTag $nicTag -DiskTag $diskTag -RecoveryAzureNetworkId $AzureVmNetworkId
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsrReplicationProtectedItem [](start = 23, length = 27)

In future we should wait for enable and IR to finish and check all these are coming fine, for now u can check in armclient #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now checked from portal. Will do it next release.


In reply to: 601630510 [](ancestors = 601630510)

$rpi = Get-AsrReplicationProtectedItem -ProtectionContainer $pc -FriendlyName $VMName

$ppgSet="/subscriptions/b364ed8d-4279-4bf8-8fd1-56f8fa0ae05c/resourceGroups/Arpita-air/providers/Microsoft.Compute/proximityPlacementGroups/h2atestppgupdate"
$currentJob = Set-AsrReplicationProtectedItem -InputObject $rpi -RecoveryProximityPlacementGroupId $ppgSet -UpdateNic $rpi.NicDetailsList[0].NicId -RecoveryNetworkId $AzureNetworkID -RecoveryNicSubnetName $subnet

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$currentJob [](start = 4, length = 11)

Please set network else in future error will come

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have set the network during enable protection. So no need to do from here.


In reply to: 601631470 [](ancestors = 601631470)

$rpi = Get-AsrReplicationProtectedItem -ProtectionContainer $pc -FriendlyName $VMName
Assert-NotNull($rpi.ProviderSpecificDetails.RecoveryProximityPlacementGroupId)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we remove, we should add check for value as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add this check. Will take care of it when I do add tests for V2A.


In reply to: 601631955 [](ancestors = 601631955)

$rpi = Get-AsrReplicationProtectedItem -ProtectionContainer $pc -FriendlyName $VMName
Assert-NotNull($rpi.ProviderSpecificDetails.RecoveryProximityPlacementGroupId)
Assert-NotNull($rpi.ProviderSpecificDetails.RecoveryVmTag)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecoveryVmTag [](start = 48, length = 13)

All assert, we should add check for value as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above


In reply to: 601632193 [](ancestors = 601632193)

[-RecoveryAvailabilitySet <String>] [-RecoveryAvailabilityZone <String>]
[-RecoveryProximityPlacementGroupId <String>] [-EnableAcceleratedNetworkingOnRecovery]
[-RecoveryBootDiagStorageAccountId <String>]
[-RecoveryAvailabilitySet <String>] [-SqlServerLicenseType <String>]
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the description based on provider #Resolved

@@ -181,6 +184,21 @@ Accept pipeline input: False
Accept wildcard characters: False
```

### -DiskTag
Specify the tags for the disks of the VM.

Copy link

@anmolbhatia289 anmolbhatia289 Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the description so that user knows for what scenarios, this is applicable. Same for all tags #Resolved

@@ -501,6 +549,22 @@ Accept pipeline input: False
Accept wildcard characters: False
```

### -SqlServerLicenseType
Specify the SQL Server license type of the VM.
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VM [](start = 43, length = 2)

this is applicable to which providers #Resolved

@Arpitaji
Copy link

Arpitaji commented Mar 25, 2021

    public const string NoLicenseType = "NoLicenseType";

We are reusing this but the link is incorrect for us #Resolved


Refers to: src/RecoveryServices/RecoveryServices.SiteRecovery/Models/PSConstants.cs:288 in 838a284. [](commit_id = 838a284, deletion_comment = False)

@@ -864,6 +868,25 @@ public ASRHyperVReplicaAzureSpecificRPIDetails(HyperVReplicaAzureReplicationDeta
/// </summary>
public string RecoveryProximityPlacementGroupId { get; set; }

/// <summary>
/// Gets or sets the SQL Server license type of the machine to in the event of a failover.
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to [](start = 68, length = 2)

"to" we need to remove #Resolved

@@ -1182,6 +1209,26 @@ public ASRInMageAzureV2SpecificRPIDetails(InMageAzureV2ReplicationDetails detail
/// Gets or sets the proximity placement group Id for replication protected item after failover.
/// </summary>
public string RecoveryProximityPlacementGroupId { get; set; }

/// <summary>
/// Gets or sets the SQL Server license type of the machine to in the event of a failover.
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to [](start = 68, length = 2)

"to" we need to remove #Resolved

}
else
{
providerSettings.TargetManagedDiskTags = this.DiskTag;
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

providerSettings.TargetManagedDiskTags = this.DiskTag [](start = 20, length = 53)

we can assign irrespective of the check like

providerSettings.TargetManagedDiskTags = this.DiskTag;
if(this.UseManagedDisk == Constants.False && this.DiskTag != null && this.DiskTag.Count > 0)
{
throw new PSArgumentException(
string.Format(
Resources.DiskTagCannotBeSet,
this.DiskTag,
this.UseManagedDisk));
}
or u can move the error above #Resolved

{
if(diskTag != null ||
diskTag.Count > 0)
{
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are checking effectively (this.DiskTag == null ||
this.DiskTag.Count == 0)

twice
something like
if (this.DiskTag == null ||
this.DiskTag.Count == 0)
{
diskTag = providerSpecificDetails.TargetManagedDiskTags;
}
else if(useManagedDisk == Constants.False)
{throw new PSArgumentException(
string.Format(
Resources.DiskTagCannotBeSet,
diskTag,
useManagedDisk));
} #Resolved

providerSettings.TargetVmTags = this.RecoveryVmTag;
providerSettings.TargetNicTags = this.RecoveryNicTag;

if(this.DiskTag != null || this.DiskTag.Count > 0)
Copy link

@Arpitaji Arpitaji Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| [](start = 36, length = 2)

&& #Resolved

@vidyadharijami
Copy link
Author

    public const string NoLicenseType = "NoLicenseType";

Shall we have another constant for Sql License type ?


In reply to: 807333660 [](ancestors = 807333660)


Refers to: src/RecoveryServices/RecoveryServices.SiteRecovery/Models/PSConstants.cs:288 in 838a284. [](commit_id = 838a284, deletion_comment = False)

this.DiskTag.Count == 0)
{
diskTag = providerSpecificDetails.TargetManagedDiskTags;
}
Copy link

@Arpitaji Arpitaji Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} [](start = 20, length = 1)

V2A edit also we had to throw error right #Resolved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it


In reply to: 602052721 [](ancestors = 602052721)

@@ -513,4 +513,7 @@ Recommended Action: To cleanup the resources created by Test failover run the Te
<data name="IPConfigNotFoundInVMNic" xml:space="preserve">
<value>IP Config "{0}" not found in VM NIC "{1}".</value>
</data>
<data name="DiskTagCannotBeSet" xml:space="preserve">
<value>Disk Tag "{0}" cannot be set if UseManagedDisk is "{1}".</value>
Copy link

@anmolbhatia289 anmolbhatia289 Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tag [](start = 16, length = 3)

Tag(s) #Resolved

Copy link

@Arpitaji Arpitaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link

@anmolbhatia289 anmolbhatia289 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@vidyadharijami vidyadharijami merged commit 2d0bec1 into newApiVersion/asr2021-02-10 Mar 26, 2021
shesamian pushed a commit that referenced this pull request Oct 21, 2022
* Upgrade to new storage dataplane SDK

* [Storage] Support dfs sas encryptionscope

* Update DependencyAnalyzer.cs (#21)

Co-authored-by: Dingmeng Xue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants